Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #463 - broken inter-window update messages #464

Closed
wants to merge 1 commit into from

Conversation

timboudreau
Copy link
Contributor

This addresses a couple of issues:

  • Messages (including repaints) not getting processed when delivered a View in a window which is inactive
  • Ability to retrieve the window id of a view without the application having to pass that information down to whatever code needs it

and addresses a related issue (related to dealing with WindowIds - the solution falls out of the solution for the above):

  • Ability to retrieve the screen position of the window if you have its id

This patch adds atracking data structure:

  • A static mapping of root-id:WindowId:Arc<Window> which is populated on window creation and cleared on destruction, which makes it possible to access Window.request_redraw() to force a redraw. It is guarded by a read-write lock (but written to only during window creation and destruction).

So, a ViewId that wants its window-id looks up its root view's id, and looks up the WindowId associated with that root.

We extend WindowId with methods to get the window bounds on screen with and without decorations and similar - these methods could be added to ViewId instead, but it seems more intuitive that you ask a window id for the window's bounds the same way you ask a view id for the view's bounds - that seems like what users will expect.

Given the usage patterns (the mapping will only be locked for writing when a window is being created or destroyed, and will not be read-locked except in the case of forcing a repaint or looking up window coordinates), it is not touched in the critical path of rendering - unless the application and destroying many windows at the same time on different threads, I would not expect the read-write lock to be contended at all in practice (famous last words, I know).

A thread-local for the mapping, as is done in update.rs might work (or look like it works), but we would be assuming no platform floem will ever support uses different event threads for different windows' event loops. I know of two that do - BeOS and Java AWT (with its stack of event threads to allow modal dialogs that work when their parent is disabled or blocked, which was a source of "interesting" deadlocks).

If there was some path that I missed that would let a ViewId reach down to the WindowHandle that owns it without doing this sort of tracking, I would love to know about it - PaintCx has this information, but it is the only thing that does, and the ViewId has no access to it.

@timboudreau timboudreau force-pushed the window_ids_clean branch 3 times, most recently from cba5e85 to 35ceb9c Compare May 28, 2024 21:58
This addresses a couple of issues:

 * Messages (including repaints) not getting processed when delivered a `View` in a window which is inactive
 * Ability to retrieve the window id of a view without the application having to pass that information down
   to whatever code needs it

and addresses a related issue - obtaining a `WindowId` for a realized `View` and its bounds on screen, in order
to convert view-relative locations into screen locations for purposes such as positioning windows relative
to a `View`.

This patch adds a tracking data structure, and a convenience data structure which contains sufficient
information to convert view-relative positions to screen-positions and back.

 * A static mapping of `root-id:WindowId`:`Arc<Window>` which is populated on window creation and cleared on destruction,
  which makes it possible to access `Window.request_redraw()` to force a redraw.  It is guarded by a read-write
  lock (but written to only during window creation and destruction).
 * `ScreenLayout` contains a snapshot of the window id, the window's content and framed bounds, the view's
  origin within the window and the monitor scaling.

So, a ViewId that wants its window-id looks up its root view's id, and looks up the WindowId associated
with that root.

We extend `WindowId` with methods to get the window bounds on screen with and without decorations and similar -
these methods could be added to `ViewId` instead, but it seems more intuitive that you ask a window id for
the window's bounds the same way you ask a view id for the view's bounds - that seems like what users will
expect.

Given the usage patterns (the mapping will only be locked for writing when a window is being created or destroyed, and will
not be read-locked except in the case of forcing a repaint or looking up window coordinates), it is not touched
in the critical path of rendering - unless the application and destroying many windows at the same time on
different threads, I would not expect the read-write lock to be contended at all in practice (famous last
words, I know).

A thread-local for the mapping, as is done in `update.rs` might work (or look like it works), but we would be
assuming no platform floem will ever support uses different event threads for different windows' event
loops. I know of two that do - BeOS and Java AWT (with its stack of event threads to allow modal dialogs
that work when their parent is disabled or blocked, which was a source of "interesting" deadlocks).

If there was some path that I missed that would let a `ViewId` reach down to the `WindowHandle` that
owns it without doing this sort of tracking, I would love to know about it - `PaintCx` has this information,
but it is the only thing that does, and the `ViewId` has no access to it.
self.add_update_message(UpdateMessage::RequestPaint);
if !active {
self.force_repaint();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need this given that we've now processing all update messages, and if there's a repaint needed, the repaint request would be sent for that window?

@timboudreau
Copy link
Contributor Author

This PR is superseded by this one (already integrated) and this one. Closing.

@timboudreau timboudreau deleted the window_ids_clean branch May 30, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants